Skip to content

feat(storage): devices, notifications and task sources on the storage ports - #7181

Merged
senamakel merged 35 commits into
tinyhumansai:mainfrom
senamakel:storage-domains
Oct 9, 2026
Merged

senamakel merged 35 commits into
tinyhumansai:mainfrom
senamakel:storage-domains

Conversation

@senamakel

@senamakel senamakel commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Paired devices, notifications and task sources on the storage ports. With a storage backend configured (OPENHUMAN_STORAGE_URL / [storage] url, feat(storage): storage backend by URL; session stores on tinystoragedrivers; bump tinyagents + tinyflows #7168), every function in security::devices::store, desktop::notifications::store and integrations::task_sources::store is served from the backend's document store instead of devices.db, notifications.db and sources.db. This follows the approvals pattern from feat(approval): approvals on the storage ports #7178.
  • Scoped to the acting agent, as approvals are: local on a single-user host, refused in SaaS mode with no acting agent.
  • Safe with several processes on one database. Notification dedup ("skip identical content from the last minute") was a SQLite BEGIN IMMEDIATE; it is now a compare-and-swap on a per-content dedup document. Device touch/revoke and task-source updates are compare-and-swap too, which also closes the read-modify-write window update_source's own comment called out.
  • A shared base, storage::documents. Repo (one domain's scoped handle: declare its collections, run a call from sync code) and compare_and_swap (the guarded-UPDATE loop). The next domains build on these instead of copying the plumbing.
  • Desktop unchanged. With no backend, the SQLite stores are used exactly as before.

Problem

Solution

  • Each domain gets a store_documents.rs beside its store.rs. Every public store function first asks store_documents::current()? and returns its answer when a backend is installed; otherwise it falls through to the existing SQLite code. Signatures are unchanged.
  • Devices: one paired_devices document per channel_id. Pairing replaces the document (the SQL INSERT OR REPLACE). Revoking an already revoked device still reports that it exists, as the SQL UPDATE does.
  • Notifications:
    • Collections integration_notifications, notification_dedup, notification_settings and core_notifications.
    • Optional fields are left out instead of stored as null, so "unscored" is a missing field on every driver.
    • received_ms orders the list; RFC 3339 strings with differing fractional digits don't sort as instants.
    • stats folds its counts in the store, since the port has no GROUP BY. It is bounded by one user's notifications.
    • A re-published core event id is ignored (Precondition::Absent).
  • Task sources:
    • Collections task_sources and ingested_tasks (id (source_id, external_id), length-prefixed so ids can't collide). filter, target and the task payload stay JSON strings.
    • Removing a source deletes its ledger entries (the SQL cascade).
    • apply_patch is extracted, so both stores apply a patch the same way.
  • Housekeeping: the SQLite row-conversion helpers moved to store/store_rows.rs in notifications and task sources, keeping both store.rs files under the 750-line layout limit.
  • Not moved, deliberately: the vault watcher's mtime state (config/workspace/state.rs). It records local file timestamps on this machine, so it belongs in local SQLite, not a shared backend.
  • Trade-off: no import of existing rows into a newly configured backend, same as approvals.

Submission Checklist

  • Tests added or updated:
    • storage/documents_tests.rs: run, error prefix, CAS apply/decline/missing, eight concurrent swaps with one winner, scope isolation.
    • devices/store_documents_tests.rs: round trip, re-pair clears revocation, touch only live devices, revoke semantics, ordering, scopes.
    • notifications/store_documents_tests.rs:
      • round trip and paging;
      • dedup within and outside the window, and eight concurrent duplicates with one insert;
      • triage, status and score filter;
      • stats, settings defaults and upsert;
      • core notifications: idempotent insert, mark read, a corrupt payload skipped;
      • scopes, and the dedup key not colliding.
    • task_sources/store_documents_tests.rs:
      • round trip and ordering;
      • patch semantics and a provider-mismatch error;
      • record_fetch;
      • the edit-aware ledger, ledger ordering, cascade on remove and clear_all;
      • a corrupt record is an error, not a panic;
      • scopes.
    • tests/storage_domains_e2e.rs: the public functions of all three with a memory backend installed, then back on SQLite after storage::clear(). It is its own test binary, like storage_approvals_e2e.
  • Diff coverage ≥ 80%: pending CI.
  • Coverage matrix updated: N/A, no user-visible feature change (the desktop path is unchanged).
  • Affected feature IDs: N/A.
  • No new external network dependencies: tests use the memory driver.
  • Manual smoke checklist: N/A, default desktop behaviour unchanged.
  • Linked issue: N/A, part of the storage-drivers migration.

Impact

  • Desktop / CLI / TUI: no change without a storage URL.
  • Cloud / SaaS: these domains persist in the configured backend, isolated per agent and safe with several replicas.
  • Dependencies: none new.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: storage-domains
  • Commit SHA: see the PR head

Validation Run

  • pnpm --filter openhuman-app format:check: N/A, no frontend changes
  • pnpm typecheck: N/A, no frontend changes
  • Focused tests: RUST_MIN_STACK=16777216 cargo test -p openhuman --lib -- integrations::task_sources desktop::notifications security::devices storage:: security::approval (395 passed); cargo test -p openhuman-cli --test storage_domains_e2e
  • Rust fmt/check (if changed): cargo fmt, cargo clippy -p openhuman --lib --tests -- -D warnings, pnpm rust:layout
  • Tauri fmt/check (if changed): N/A

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: with a storage backend configured, these three domains persist there, scoped per agent.
  • User-visible effect: none on the desktop.

Parity Contract

  • Legacy behavior preserved: every public store signature and return contract is unchanged; the SQLite path is untouched when no backend is installed.
  • Guard/fallback/dispatch parity checks: the document stores mirror the SQL semantics (INSERT OR REPLACE, update-matched-a-row booleans, the 60 s dedup window, limit.max(1), cascade on delete, corrupt rows skipped where SQL skipped them), and the tests assert them.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): none
  • Canonical PR: this one
  • Resolution (closed/superseded/updated): N/A

Summary by CodeRabbit

  • New Features
    • Notifications, task sources, ingested tasks, and paired devices can now use a configured storage backend, with data kept separate by storage scope.
    • Existing local database storage remains available when no backend is configured.
  • Documentation
    • Added guidance on storage behavior for notifications, task sources, paired devices, and shared document storage.

senamakel and others added 25 commits October 9, 2026 12:18
Reworked the document storage code to reduce duplication and make the
read and write paths easier to follow. Behaviour is unchanged.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Register the documents storage module so its API is reachable from the
storage crate root.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Device store operations now delegate to the document port when a storage
backend is configured, falling back to the existing SQLite path otherwise.
This lets paired-device persistence work under document storage without
changing callers.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformat the device store and document storage modules and their tests
to satisfy rustfmt, wrapping long call chains and assertions. No
behavioural change.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds a store_documents module to the desktop notifications layer so
notification documents can be persisted and retrieved.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Every notification store function now delegates to the document port when a
storage backend is configured, falling back to the existing SQLite path
otherwise. The document list filter was also rebuilt from optional clauses so
provider and score constraints compose correctly.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add the store_documents module to the notifications module tree so its contents are compiled and available to the rest of the crate.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformat the received_ms document insert in the notification store to
multi-line form, matching the surrounding style. No behaviour change.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The `set` helper now requires its edit closure to be `Sync` in addition to
`Send`, so callers can share the closure across threads when applying
notification document edits.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Replace the default version with the explicit first version constant in the unparseable raw payload test so the fixture states the version it intends rather than relying on the default.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
…hrough store

Split the patch logic out of update_source into a shared apply_patch helper so
both the SQLite and document stores can reuse it. update_source now delegates to
the document store when one is active, falling back to the existing SQLite path
otherwise.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The implementation note about the three SQLite connections and the TOCTOU
window was attached to the wrong function, so it now sits on update_source
where it applies. The note also records that the document store applies the
patch under compare-and-swap and therefore has no such window.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a task source that reads tasks from stored documents, letting the
integration layer surface document-backed work items alongside existing
sources.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Dropped the private TaskSourceStorageError marker and its From<StorageError>
impl, which were no longer referenced since storage errors surface through
Repo::run. Also trimmed the now-unused StorageError import.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Each store function now checks for a configured storage backend and delegates to the document port when one is present, falling back to the existing SQLite path otherwise. This lets task sources and the ingested ledger be served from the document store without changing callers.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add the store_documents module declaration so the new submodule is compiled as part of task_sources.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted the task source document store and its tests to satisfy
rustfmt, wrapping long signatures, calls, and assertions. The Notion
filter spec in the tests also gained the new assigned_to_me and status
fields so it matches the current type.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The provider slug parse error was not convertible into the anyhow error
returned by `to_source`, so the `?` operator failed to compile. The error
is now explicitly mapped into an anyhow error before propagation.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Moved the notification row-to-struct conversion helpers out of store.rs into a dedicated store_rows submodule, leaving the store to import rows_to_notifications. Also qualified ScopedStorage paths inline in the document stores to drop now-unused imports.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Moved the task source row mapping, timestamp parsing, and SQL conversion
helpers out of store.rs into a dedicated store_rows submodule, matching the
layout already used by the notifications store. Also applied rustfmt to the
notification row helper signature.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Removed the unused DateTime import from the notification store, leaving only Utc which is still in use.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a dedicated test target for the storage domains end-to-end suite so it
runs in its own binary, matching storage_approvals_e2e, since it installs a
storage backend into the process-wide slot.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
… and devices

Added README sections describing how each module's store functions switch to the tinystoragedrivers document port when a storage backend is configured, covering collections, scoping, and the compare-and-swap semantics that replace the SQLite transactions. The desktop default of using the local database files is noted in each case.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds the scope accessors, the block_on runtime bridge and the documents::Repo
and compare_and_swap primitives to the storage README, and lists the domain
stores that switch to the document port when a backend is installed.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d9a1c834-184a-4ac4-9c47-f44fdf54f62d

📥 Commits

Reviewing files that changed from the base of the PR and between c76d5c2 and 512275f.


📒 Files selected for processing (24)
  • crates/openhuman-cli/Cargo.toml
  • crates/openhuman-core/src/desktop/notifications/README.md
  • crates/openhuman-core/src/desktop/notifications/mod.rs
  • crates/openhuman-core/src/desktop/notifications/rpc.rs
  • crates/openhuman-core/src/desktop/notifications/store.rs
  • crates/openhuman-core/src/desktop/notifications/store/store_rows.rs
  • crates/openhuman-core/src/desktop/notifications/store_documents.rs
  • crates/openhuman-core/src/desktop/notifications/store_documents_tests.rs
  • crates/openhuman-core/src/integrations/task_sources/README.md
  • crates/openhuman-core/src/integrations/task_sources/mod.rs
  • crates/openhuman-core/src/integrations/task_sources/store.rs
  • crates/openhuman-core/src/integrations/task_sources/store/store_rows.rs
  • crates/openhuman-core/src/integrations/task_sources/store_documents.rs
  • crates/openhuman-core/src/integrations/task_sources/store_documents_tests.rs
  • crates/openhuman-core/src/security/devices/README.md
  • crates/openhuman-core/src/security/devices/mod.rs
  • crates/openhuman-core/src/security/devices/store.rs
  • crates/openhuman-core/src/security/devices/store_documents.rs
  • crates/openhuman-core/src/security/devices/store_documents_tests.rs
  • crates/openhuman-core/src/storage/README.md
  • crates/openhuman-core/src/storage/documents.rs
  • crates/openhuman-core/src/storage/documents_tests.rs
  • crates/openhuman-core/src/storage/mod.rs
  • tests/storage_domains_e2e.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.



📝 Walkthrough

Walkthrough

The change adds shared document-storage utilities and document-backed implementations for notifications, task sources, and paired devices. Each domain selects the document backend when configured and retains its SQLite path otherwise. Tests cover storage operations, concurrency, and scope isolation.

Changes

Scoped storage foundation

Layer / File(s) Summary
Document repository and guarded updates
crates/openhuman-core/src/storage/{README.md,mod.rs,documents.rs,documents_tests.rs}
Adds scoped repository access, collection setup, a blocking bridge, and compare-and-swap with conflict retries. Tests cover operation results, conflicts, concurrency, and scope isolation.

Notification storage

Layer / File(s) Summary
Notification document storage and SQLite routing
crates/openhuman-core/src/desktop/notifications/*
Adds document-backed integration and core notifications, including deduplication, settings, statistics, workspace-scoped events, and read/status updates. Store operations continue to use SQLite when no document backend is configured. The change also moves SQLite row conversion into store_rows and uses spawn_scoped for background triage.

Task-source storage

Layer / File(s) Summary
Task-source and ingestion document storage
crates/openhuman-core/src/integrations/task_sources/*
Adds document-backed source and ingestion-ledger operations, including source updates, fetch records, payload listing, and ledger cleanup on source removal. Store operations retain SQLite fallbacks, with row conversion moved into store_rows.

Paired-device storage

Layer / File(s) Summary
Paired-device document storage
crates/openhuman-core/src/security/devices/*
Adds document-backed insertion, touch, revocation, retrieval, and listing. Store operations retain SQLite fallbacks.

Cross-domain verification

Layer / File(s) Summary
Storage-domain integration test
crates/openhuman-cli/Cargo.toml, tests/storage_domains_e2e.rs
Adds a separate test binary that exercises device, notification, and task-source stores with an in-memory backend, then checks store behavior after the backend is cleared.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Feature

Suggested reviewers: codeghost21


Merge Risk

Merge Risk: ⚪ Minimal · up to 51227

No confirmed material issue currently prevents merging after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 51227

Agent-scoped storage improves isolation, but important lifecycle guarantees remain unresolved. Pairing callbacks do not explicitly carry the identity required by the new store, and concurrent task writes can outlive source deletion. These issues are bounded by existing controls, but warrant attention before relying on the shared backend.

Retained concerns

  • Medium · security · inferred: Pairing persistence newly requires an ambient acting agent, but the global tunnel subscriber neither captures nor passes the registering owner. Without that context, SaaS persistence is refused after the V2 cipher may already have been installed and acknowledged; the failure is only logged. This can leave a connected device outside the owner's persisted inventory. Callback context propagation remains unverified. Explicit revocation by channel still clears local cipher state even when no record exists, so inability to revoke or cross-agent access is not established.
  • Low · security · inferred: A ledger writer can pass the source-existence check, then write task title and payload after source removal has deleted both the ledger and its parent. The new document operations therefore permit retained data without its owning source, unlike the base SQLite foreign key and cascading deletion. Exposure remains within the resolved agent scope; cross-agent access and same-ID recreation through normal source creation are not established. The current fetch producer is unavailable, so a live ingestion attack path was not demonstrated.

Security review details

Security Blast Radius

  • inferred — The identified persistence failures concern paired-device inventory and task payloads within resolved storage scopes. Missing SaaS identity fails closed at storage rather than selecting a shared bucket. No demonstrated path establishes cross-agent reads, backend credential compromise, or infrastructure privilege escalation.

Security Findings and Attack Paths

  • inferred — The conditional device path is a valid pending-channel handshake, followed by cipher activation, then a refused ownerless persistence call. Activation-before-persistence and best-effort error handling predate this PR; the new acting-agent requirement introduces an additional failure condition. This is not evidence that an unpaired attacker can authenticate.

Trust Boundaries and Controls

  • observed — Device touch cannot revive a revoked document, and explicit revoke marks the record and clears pending sessions, keys, peer status, and active ciphers in the current process. These are meaningful countercontrols, but do not prove asynchronous owner propagation or revocation across every process sharing the backend.

Resilience and Maintainability Implications

  • observed — Ledger entries contain task titles and serialized payloads, and ledger listing does not require a surviving source. Consequently, restoring the parent-deletion invariant matters for data lifecycle and cleanup, not merely deduplication. The current unavailable fetch producer limits demonstrated operational reach.

Hardening Proposals

  • proposed — Bind the registering agent to the pending pairing state and restore that ownership explicitly in the callback. Make successful persistence part of activation, or remove installed cipher state when persistence fails.
  • proposed — Coordinate source deletion with ledger writers using an atomic backend operation or a deletion lifecycle that rejects late writes and completes cleanup during recovery. A separate existence check alone cannot preserve the former foreign-key guarantee.



🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 57.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 155 functions across 19 files. (5 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely identifies the main change: routing device, notification, and task-source storage through storage ports.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 57.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 155 functions across 19 files. (5 skipped: 5 unsupported.)



  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the scoped-store trail,
While documents hop where backends prevail.
Devices, tasks, and notices land,
With SQLite waiting close at hand.
The tests nibble through each case,
And leave clean tracks in every place.

Comment @coderabbitai help to get the list of available commands.

@senamakel senamakel changed the title Storage domains feat(storage): devices, notifications and task sources on the storage ports Oct 9, 2026
@senamakel
senamakel marked this pull request as ready for review October 9, 2026 10:37
@tinysweeper

tinysweeper Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper reviewed this change across 6 lane(s) and found 20 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below.

State: Changes requested
Priority: critical
Reviewed head: 512275f47d56
Updated: 1791545800 (Unix time)

Review snapshot

Change surface Files Review signal Count
Production 14 Active findings 38
Tests 5 Noted findings 0
Documentation 4 Resolved findings 139
Configuration 1 Pending checks/questions 4

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

The review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below.

Features

None identified with supported citations.

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

  • critical · critique · Add the declared store_rows module — This declaration has no corresponding `store_rows.rs` module in the repository, so the crate fails to compile with a missing module file. Add the module source at the expected path (crates/openhuman\-core/src/integrations/task\_sources/store\.rs:445)
  • critical · critique · Add the declared test module source — The new file declares `store_documents_tests.rs` under `#[cfg(test)]`, but that source file is not included in the supplied patch. If it does not already exist in the branch, any t (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:519)
  • critical · critique · Add the missing store_rows module source — Rust resolves this declaration to `store_rows.rs` or `store_rows/mod.rs` beside `store.rs`. Neither source file is present in the complete diff, so this change fails to compile unl (crates/openhuman\-core/src/desktop/notifications/store\.rs:704)
  • high · critique · Do not release another process's dedup claim — The rollback identifies ownership only by `last_ms`. Two callers can receive the same millisecond from `Utc::now().timestamp_millis()`; if the second caller claims the same content (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:203)
  • high · critique · Do not leave a dedup claim before storing the notification — The dedup document is claimed before the notification document is written. If the process exits or loses its storage connection between these operations, the claim remains for up t (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:248)
  • high · critique · Record the actual arrival time for deduplication — `now_ms` is captured before entering the document-store operation. Under executor or storage contention, the claim can be written substantially later, but its timestamp still refle (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:246)
  • medium · critique · Do not treat malformed notification timestamps as valid state — A corrupt `received_at` is silently replaced with the current time, changing the notification's ordering and making the corruption invisible to callers. A corrupt `scored_at` is si (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:132)
  • medium · critique · Preserve full timestamp ordering when listing sources — `created_ms` truncates `DateTime<Utc>` to milliseconds. Two sources created within one millisecond are then ordered by `_id`, not by their actual creation time, so `list_sources` c (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:39)
  • medium · critique · Preserve full timestamp ordering for ingested references — `ingested_ms` truncates the supplied ingestion time to milliseconds. References ingested within the same millisecond are ordered by `external_id`, which is not their arrival order. (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:40)
  • medium · critique · Reject unknown notification statuses — Any unknown or malformed stored status, including a typo such as `"dismiss"` or a non-string value, is silently mapped to `Unread`. That makes corrupted or newly introduced states (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:73)
  • medium · critique · Make source removal and ledger cleanup atomic — These are two independent writes. If the source deletion fails after the ledger deletion succeeds, the source remains but all of its deduplication history is gone; a retry can re-i (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:199)
  • medium · critique · Make the source check and ledger insert atomic — The parent check and the subsequent `put` are separate operations. If source removal runs after `get_source` succeeds but before `docs.put`, this method inserts an orphan ledger en (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:264)
  • medium · critique · Make clearing sources and ledger entries atomic — The two collection-wide deletes are independent. If the second delete fails after the source delete succeeds, the reset leaves all ingestion ledger entries behind; if a later opera (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:339)
  • medium · critique · Propagate a missing source from record_fetch — `record_fetch` updates an existing source, so a nonexistent source should be reported as an error rather than silently accepted. This test currently requires the opposite behavior (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\_tests\.rs:130)
  • medium · critique · Propagate a missing source from record_fetch — `compare_and_swap` returns no updated document when the source does not exist, but this code maps that result directly to `Ok(())`. A fetch racing with source removal therefore rep (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:220)
  • critical · security · Add the missing `store_rows` module source — This declaration requires `store_rows.rs` (or an inline module body), but the pull request does not provide that source. The crate therefore fails to compile. Add the module contai (crates/openhuman\-core/src/desktop/notifications/store\.rs:704)
  • critical · security · Add the declared store_rows module — This declaration requires `crates/openhuman-core/src/integrations/task_sources/store_rows.rs` (or an inline module), but no such source is present in the provided pull-request diff (crates/openhuman\-core/src/integrations/task\_sources/store\.rs:445)
  • high · security · Store the notification before claiming its deduplication entry — The deduplication document is updated before the notification document is inserted. A process crash between these operations permanently suppresses a notification for the deduplica (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:248)
  • medium · security · Propagate a missing source from record_fetch — `compare_and_swap` returns `None` when the source does not exist, but the result is mapped to `()` and reported as success. Callers can therefore record a fetch for a deleted or un (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:220)
  • medium · security · Require record_fetch to reject a missing source — This test explicitly accepts `record_fetch` succeeding for an unknown source, which codifies the missing-source behavior previously identified as incorrect. A fetch status must not (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\_tests\.rs:130)
  • medium · security · Reject unknown notification statuses — Malformed or future status values are silently converted to `Unread`. This makes corrupted data appear actionable and can disagree with `unread_count`, which only counts documents (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:78)
  • medium · security · Do not replace invalid receipt times with the current time — A missing or malformed persisted `received_at` is replaced with the current time during reads. That changes historical notification metadata and can make old corrupt records appear (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:132)
  • medium · security · Preserve full timestamp ordering when listing sources — Sources are persisted with only epoch milliseconds, so sources created within the same millisecond are ordered by `_id` rather than their actual `DateTime<Utc>` values. This can ch (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:164)
  • medium · security · Preserve full timestamp ordering for ingested references — The reference listing sorts only the millisecond timestamp and then the external ID. Multiple ingestions can share that millisecond while having different actual arrival times, so (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:285)
  • medium · security · Preserve full ingestion ordering when listing tasks — Recently ingested tasks are ordered only by epoch milliseconds. Tasks received in the same millisecond are therefore ordered by the backend's unspecified tie behavior, which can vi (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:313)
  • medium · security · Do not treat malformed scoring timestamps as unscored — When `scored_at` is present but malformed, `parse_time` returns `None`, making the notification look unscored even though it has persisted scoring fields. This creates incorrect tr (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:133)
  • medium · security · Make the source check and ledger write atomic — The parent existence check is performed in a separate operation from the ledger write. A concurrent source removal can pass `get_source`, delete the source and its ledger, and then (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:264)
  • medium · tests · Avoid sleeping to establish timestamp ordering — Still unfixed from the earlier review: `devices_list_oldest_first` relies on a 5 ms sleep to make `created_at` differ, which is a time-based flake the repository rules forbid. Inje (crates/openhuman\-core/src/security/devices/store\_documents\_tests\.rs:74)
  • medium · tests · Do not claim the dedup key on a plain insert — `insert` is the non-dedup path (`insert_if_not_recent` is the dedup one), but it calls `claim_content` unconditionally, so even with `skip_recent = false` it stamps `last_ms = now` (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:248)
  • medium · tests · Migrate existing pairings before switching storage backends — Still unfixed from the earlier review: the moment a backend is configured, every read and write goes to the document port and anything already persisted in `devices.db` — paired de (crates/openhuman\-core/src/security/devices/store\.rs:30)
  • medium · tests · Make source removal and ledger cleanup atomic — Still unfixed from the earlier review: the ledger wipe and the source delete are two independent operations. The ordering comment explains the chosen failure mode, but a crash betw (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:199)
  • medium · description · Do not treat malformed scoring times as unscored — The row conversion moved to `store_rows.rs` unchanged, so the finding stands: a stored but unparseable `scored_at` is reported as never scored, losing the fact that triage ran. An (\(pull request description\))
  • medium · e2e · Do not treat malformed scoring times as unscored — Still stands from the earlier revision and unchanged: a `scored_at` value that fails to parse makes the notification read as unscored, so the intelligence pipeline re-scores conten (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:133)
  • medium · e2e · Do not replace invalid receipt times with the current time — Still stands: an unparseable `received_at` becomes the current time, silently reordering and re-deduplicating against real arrivals. Surface the corruption rather than fabricating (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:132)
  • medium · e2e · Reject unknown notification statuses — Still stands: an unrecognized `status` string silently reads as `Unread`, corrupting counts and re-surfacing dismissed or acted notifications. Fail the read (or log loudly and surf (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs:78)
  • medium · e2e · Make source removal and ledger cleanup atomic — Still stands: ledger deletion and source deletion are two separate operations, so a failure between them deletes ledger entries for a source that still exists (or, reversed, orphan (crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs:199)

Previously reported and still active

  • Add the declared store\_rows module
  • Propagate a missing source from record\_fetch

Resolved this pass

  • Add the declared store_rows module
  • critical — Add the declared store_rows module
  • critical — Add the missing store_rows module source
  • critical — Add the test source or remove the target
  • high — Record the actual arrival time for deduplication
  • medium — Preserve full ingestion ordering when listing tasks
  • medium — Avoid sleeping to establish timestamp ordering
  • medium — Preserve full timestamp ordering when listing sources
  • medium — Preserve full timestamp ordering for ingested references
  • medium — Migrate existing pairings before switching storage backends
  • medium — Skip the SQLite interval bound for document-backed sources
  • medium — Make source removal and ledger cleanup atomic
  • medium — Propagate a missing source from record_fetch
  • medium — Avoid sleeping to establish persistence ordering
  • medium — Bound dedup timestamps to the actual arrival time
  • Skip the SQLite interval bound for document-backed sources
  • critical — Add the declared store_rows module
  • critical — Add the missing store_rows module source
  • critical — Add the test source or remove the target
  • medium — Avoid sleeping to establish persistence ordering
  • medium — Bound dedup timestamps to the actual arrival time
  • medium — Propagate malformed ingested task payloads
  • Add the declared store_rows module
  • Do not consume the dedup claim before storing the notification
  • Record the actual arrival time for deduplication
  • Do not treat malformed scoring times as unscored
  • Preserve full ingestion ordering when listing tasks
  • Avoid sleeping to establish timestamp ordering
  • Preserve full timestamp ordering when listing sources
  • Preserve full timestamp ordering for ingested references
  • Do not replace invalid receipt times with the current time
  • Propagate malformed ingested task payloads
  • Migrate existing pairings before switching storage backends
  • Do not treat malformed scoring timestamps as unscored
  • Reject unknown notification statuses
  • Skip the SQLite interval bound for document-backed sources
  • Propagate a missing source from record_fetch
  • Make source removal and ledger cleanup atomic
  • Add the test source or remove the target
  • Add the missing store_rows module source
  • Avoid sleeping to establish persistence ordering
  • Bound dedup timestamps to the actual arrival time
  • Add the declared store_rows module
  • Do not consume the dedup claim before storing the notification
  • Record the actual arrival time for deduplication
  • Do not treat malformed scoring times as unscored
  • Avoid sleeping to establish timestamp ordering
  • Do not replace invalid receipt times with the current time
  • Propagate malformed ingested task payloads
  • Migrate existing pairings before switching storage backends
  • Do not treat malformed scoring timestamps as unscored
  • Reject unknown notification statuses
  • Skip the SQLite interval bound for document-backed sources
  • Add the test source or remove the target
  • Add the missing store_rows module source
  • Avoid sleeping to establish persistence ordering
  • Bound dedup timestamps to the actual arrival time
  • critical — Add the test source or remove the target
  • critical — Add the declared store_rows module
  • critical — Add the missing store_rows module source
  • Record the actual arrival time for deduplication
  • Avoid sleeping to establish timestamp ordering
  • Bound dedup timestamps to the actual arrival time
  • high — Do not consume the dedup claim before storing the notification
  • high — Record the actual arrival time for deduplication
  • critical — Add the test source or remove the target
  • Do not treat malformed scoring times as unscored
  • Preserve full ingestion ordering when listing tasks
  • Avoid sleeping to establish timestamp ordering
  • Preserve full timestamp ordering when listing sources
  • Preserve full timestamp ordering for ingested references
  • Do not replace invalid receipt times with the current time
  • Propagate malformed ingested task payloads
  • Migrate existing pairings before switching storage backends
  • Do not treat malformed scoring timestamps as unscored
  • Reject unknown notification statuses
  • Skip the SQLite interval bound for document-backed sources
  • Propagate a missing source from record_fetch
  • Make source removal and ledger cleanup atomic
  • Propagate the missing source from record_fetch
  • Avoid sleeping to establish persistence ordering
  • Bound dedup timestamps to the actual arrival time
  • Add the declared store_rows module
  • Do not consume the dedup claim before storing the notification
  • Record the actual arrival time for deduplication
  • Do not treat malformed scoring times as unscored
  • Preserve full ingestion ordering when listing tasks
  • Avoid sleeping to establish timestamp ordering
  • Preserve full timestamp ordering when listing sources
  • Preserve full timestamp ordering for ingested references
  • Do not replace invalid receipt times with the current time
  • Propagate malformed ingested task payloads
  • Migrate existing pairings before switching storage backends
  • Do not treat malformed scoring timestamps as unscored
  • Reject unknown notification statuses
  • Skip the SQLite interval bound for document-backed sources
  • Propagate a missing source from record_fetch
  • Make source removal and ledger cleanup atomic
  • Add the test source or remove the target
  • Add the missing store_rows module source
  • Avoid sleeping to establish persistence ordering
  • Bound dedup timestamps to the actual arrival time
  • Add the declared store_rows module
  • Add the declared store_rows module
  • Add the missing store_rows module source
  • Add the test source or remove the target
  • Do not consume the dedup claim before storing the notification
  • Record the actual arrival time for deduplication
  • Bound dedup timestamps to the actual arrival time
  • Propagate malformed ingested task payloads
  • Skip the SQLite interval bound for document-backed sources
  • Preserve full timestamp ordering when listing sources
  • Preserve full timestamp ordering when listing sources
  • Preserve full ingestion ordering when listing tasks
  • Preserve full timestamp ordering for ingested references
  • Avoid sleeping to establish persistence ordering
  • Add the declared store_rows module
  • Add the declared store_rows module
  • Add the missing store_rows module source
  • Add the test source or remove the target
  • Do not consume the dedup claim before storing the notification
  • Record the actual arrival time for deduplication
  • Bound dedup timestamps to the actual arrival time
  • Skip the SQLite interval bound for document-backed sources
  • Preserve full timestamp ordering when listing sources
  • Preserve full timestamp ordering for ingested references
  • Propagate malformed ingested task payloads
  • Add the declared store_rows module
  • Add the declared store_rows module
  • Add the missing store_rows module source
  • Add the test source or remove the target
  • Do not consume the dedup claim before storing the notification
  • Record the actual arrival time for deduplication
  • Bound dedup timestamps to the actual arrival time
  • Preserve full ingestion ordering when listing tasks
  • Preserve full timestamp ordering when listing sources
  • Preserve full timestamp ordering for ingested references
  • Skip the SQLite interval bound for document-backed sources
  • Propagate malformed ingested task payloads

Pending checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS)

Before merge

  • Address carried finding Add the declared store\_rows module.
  • Address carried finding Propagate a missing source from record\_fetch.
  • Address Add the declared store_rows module (crates/openhuman\-core/src/integrations/task\_sources/store\.rs).
  • Address Add the declared test module source (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs).
  • Address Add the missing store_rows module source (crates/openhuman\-core/src/desktop/notifications/store\.rs).
  • Address Do not release another process's dedup claim (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs).
  • Address Do not leave a dedup claim before storing the notification (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs).
  • Address Record the actual arrival time for deduplication (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs).
  • Address Add the missing `store_rows` module source (crates/openhuman\-core/src/desktop/notifications/store\.rs).
  • Address Add the declared store_rows module (crates/openhuman\-core/src/integrations/task\_sources/store\.rs).
  • Address Store the notification before claiming its deduplication entry (crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs).
  • Wait for Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS).

How this fits together

flowchart LR
  n0["handle_ingest<br/>changed"]:::changed
  n1["exists_recent<br/>changed<br/>2 findings"]:::blocking
  n2["get_settings<br/>changed<br/>2 findings"]:::blocking
  n3["insert<br/>changed<br/>2 findings"]:::blocking
  n4["list<br/>changed<br/>2 findings"]:::blocking
  n5["list_core_notifications<br/>changed<br/>2 findings"]:::blocking
  n6["mark_acted<br/>changed<br/>2 findings"]:::blocking
  n7["with_connection"]:::impacted
  n8["with_connection"]:::impacted
  n9["prepare"]:::impacted
  n10["format"]:::impacted
  n0 -->|calls| n10
  n1 -->|calls| n7
  n2 -->|calls| n7
  n2 -->|calls| n9
  n3 -->|calls| n7
  n4 -->|calls| n7
  n4 -->|calls| n9
  n4 -->|calls| n10
  n5 -->|calls| n7
  n5 -->|calls| n9
  n6 -->|calls| n7
  n7 -->|calls| n10
  n8 -->|calls| n10
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Loading
Agent review details

critique

  • Conclusion: Failure
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 8 files; 26 findings. (3 already reported on an earlier push) (2 earlier finding(s) still open) (5 observation(s) grouped into shared inline comments) (+8 more not shown) _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\.rs — Add the declared store_rows module
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Add the declared test module source
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\.rs — Add the missing store_rows module source
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Do not release another process's dedup claim
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Do not leave a dedup claim before storing the notification
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Record the actual arrival time for deduplication
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Do not treat malformed notification timestamps as valid state
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Preserve full timestamp ordering when listing sources
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Preserve full timestamp ordering for ingested references
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Reject unknown notification statuses
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Make source removal and ledger cleanup atomic
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Make the source check and ledger insert atomic
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Make clearing sources and ledger entries atomic
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\_tests\.rs — Propagate a missing source from record_fetch
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Propagate a missing source from record_fetch

security

  • Conclusion: Failure
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 8 files; 14 findings. (2 earlier finding(s) still open) (2 observation(s) grouped into shared inline comments) (+2 more not shown) _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\.rs — Add the missing `store_rows` module source
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\.rs — Add the declared store_rows module
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Store the notification before claiming its deduplication entry
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Propagate a missing source from record_fetch
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\_tests\.rs — Require record_fetch to reject a missing source
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Reject unknown notification statuses
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Do not replace invalid receipt times with the current time
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Preserve full timestamp ordering when listing sources
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Preserve full timestamp ordering for ingested references
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Preserve full ingestion ordering when listing tasks
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Do not treat malformed scoring timestamps as unscored
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Make the source check and ledger write atomic

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The revision adds document-port backends for devices, notifications and task sources beside their SQLite stores, and now ships the previously missing `store_rows` modules, the dedup claim/rollback, arrival-time-based deduplication, the e2e test target and its Cargo entry, so most earlier findings are resolved. What remains: the notifications and devices stores still swallow malformed timestamps and unknown statuses, the devices test still sleeps, there is no data migration when a backend is first configured, removal and ledger cleanup are still not atomic, and a new divergence — the document-backed plain `insert` claims the dedup key even when dedup is not requested — can suppress later notifications. These are merge-blocking only in aggregate; the new backend logic itself is well tested. (1 finding discarded for not matching a changed line) (4 earlier finding(s) still open) (4 observation(s) grouped into shared inline comments) (+3 more not shown) _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._
  • Evidence: crates/openhuman\-core/src/security/devices/store\_documents\_tests\.rs — Avoid sleeping to establish timestamp ordering
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Do not claim the dedup key on a plain insert
  • Evidence: crates/openhuman\-core/src/security/devices/store\.rs — Migrate existing pairings before switching storage backends
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Make source removal and ledger cleanup atomic

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The revision lands the three domain stores on the document port and fixes nearly every earlier finding: the store_rows modules exist, the dedup claim is released on a failed insert and uses the wall clock, the SQLite-only interval bound is skipped on the document path, ledger orderings have tiebreakers, and the e2e target is declared. A handful of earlier medium findings still stand (they were carried over into the new/moved code rather than fixed), and one small new semantic drift appeared: a plain insert now refreshes the dedup claim. (4 earlier finding(s) still open) (1 observation(s) grouped into shared inline comments) (+7 more not shown) _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._
  • Evidence: \(pull request description\) — Do not treat malformed scoring times as unscored

e2e

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: This revision lands the document-port stores for notifications, task sources and paired devices, and the new tests/storage_domains_e2e.rs (with its Cargo.toml [[test]] entry) does drive the main backend-backed paths end to end for all three domains plus the fallback to SQLite after storage::clear(). The earlier store_rows/dedup-arrival/ordering/payload findings are fixed, but the malformed-timestamp, unknown-status, sleep and non-atomic-removal concerns still stand, and the document-backed core-notification, status-transition, settings, touch/update/remove paths have no end-to-end driver. Waiting on end-to-end jobs: `Rust E2E (mock backend)`, `Build Playwright E2E Artifact`, `E2E (Playwright / web lane)`, `Desktop E2E (full suite, 3 OS)`. (5 earlier finding(s) still open) (4 observation(s) grouped into shared inline comments) (+3 more not shown)
  • Unresolved questions/checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS)
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Do not treat malformed scoring times as unscored
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Do not replace invalid receipt times with the current time
  • Evidence: crates/openhuman\-core/src/desktop/notifications/store\_documents\.rs — Reject unknown notification statuses
  • Evidence: crates/openhuman\-core/src/integrations/task\_sources/store\_documents\.rs — Make source removal and ledger cleanup atomic
Evidence and run details
  • Models: gpt-5.6-luna, glm-5.3-flash
  • Spend: $0.042229
  • Tokens: 1132412 input · 86071 output · 77495 cached · 0 embedding
Head State Pass summary
628e1e068746 changes requested 20 active finding(s), 0 resolved finding(s) (at 1791542982)
512275f47d56 changes requested 36 active finding(s), 139 resolved finding(s) (at 1791545800)

tinysweeper 0.1.0

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-09T11:34:19.815772Z 512275f New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 628e1e0687

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/openhuman-core/src/desktop/notifications/store.rs
Comment thread crates/openhuman-core/src/desktop/notifications/store.rs Outdated
Comment thread crates/openhuman-core/src/desktop/notifications/store_documents.rs Outdated

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: 2 lane(s) blocking, worst finding is critical.

Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.

             $0.0302 · 1,350,172 in / 62,174 out · 98,558 cached (7%)  · gpt-5.6-luna, glm-5.3-flash
critique:    $0.0225 · 687,884 in   / 37,165 out · 47,677 cached (7%)  · gpt-5.6-luna, glm-5.3-flash
security:    $0.0061 · 496,794 in   / 21,358 out · 50,881 cached (10%) · gpt-5.6-luna
tests:       $0.0004 · 39,295 in    / 111 out    · 0 cached (0%)       · glm-5.3-flash
description: $0.0004 · 40,129 in    / 82 out     · 0 cached (0%)       · glm-5.3-flash
e2e:         $0.0004 · 42,749 in    / 207 out    · 0 cached (0%)       · glm-5.3-flash

Comment thread crates/openhuman-core/src/desktop/notifications/store.rs
Comment thread crates/openhuman-core/src/desktop/notifications/store_documents.rs Outdated
Comment thread crates/openhuman-core/src/desktop/notifications/store_documents.rs Outdated
Comment thread crates/openhuman-core/src/desktop/notifications/store/store_rows.rs
Comment thread crates/openhuman-core/src/desktop/notifications/store/store_rows.rs
Comment thread crates/openhuman-core/src/integrations/task_sources/store.rs
Comment thread crates/openhuman-core/src/integrations/task_sources/store_documents.rs Outdated
Comment thread crates/openhuman-cli/Cargo.toml
@tinysweeper tinysweeper Bot added the priority: p0 Drop what you are doing. Data loss, a live break, or an exploitable hole. label Oct 9, 2026
senamakel and others added 9 commits October 9, 2026 14:07
Reformatted a multi-line assert! call in the storage domains end-to-end test into a single line. No behaviour change.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a store_documents module to the desktop notifications subsystem so
notification documents can be persisted and retrieved.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
…tore

Moved the document persistence logic into its own module so the
notification store no longer mixes document handling with its other
responsibilities. Behaviour is unchanged.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds tests exercising the notification store document handling to lock in
current behaviour and guard against regressions.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds coverage for the document store notification RPC handlers, exercising
the store-backed paths that previously had no tests. This makes regressions
in document notification delivery easier to catch.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds a store_documents module and wires it into the task sources store so
documents can be persisted alongside tasks. This gives task sources a place
to keep document data without overloading the existing task storage.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a store for persisting task source documents, along with tests covering its behaviour.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted the notification store and its tests, plus a task source
test, to satisfy rustfmt line-width rules. No behaviour changed.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Removed a redundant Ok wrapper and trailing ? in the ingested-documents query, returning the collected result directly. No behaviour change.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 512275f47d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


/// The document store for this call, when the host configured one.
pub(super) fn current() -> Result<Option<Docs>> {
Ok(Repo::current(DOMAIN, collections)?.map(Docs))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Carry scope into background notification writes

When a storage backend is used with derived agents, changing handle_ingest to spawn_scoped does not cover the process-wide NotificationBridgeSubscriber::handle or cron::scheduler::push_cron_alert, both of which enter this helper without the originating agent's task-local CoreContext. Their writes therefore go to local in non-SaaS mode or fail with "no acting agent" in SaaS, while list/mark RPCs read the agent bucket, so core and cron alerts are not durably visible to that agent; these producers need to carry an explicit owner scope.

AGENTS.md reference: AGENTS.md:L401-L408

Useful? React with 👍 / 👎.


/// The document store for this call, when the host configured one.
pub(super) fn current() -> Result<Option<Docs>> {
Ok(Repo::current(DOMAIN, collections)?.map(Docs))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Persist pairings in the requester's agent scope

When devices_create_pairing runs under a derived agent, the actual insert happens later in DeviceTunnelSubscriber::handle_tunnel_frame after a process-wide socket/bus event, and PairingSession retains no CoreContext or storage scope. Resolving current() here consequently writes the pairing to local in non-SaaS mode or errors in SaaS, so the cryptographic handshake can complete while devices_list for the requesting agent never sees a persisted device; retain the requester's scope with the pending pairing and use it for the insert.

AGENTS.md reference: AGENTS.md:L401-L408

Useful? React with 👍 / 👎.


/// The document store for this call, when the host configured one.
pub(super) fn current() -> Result<Option<Docs>> {
Ok(Repo::current(DOMAIN, collections)?.map(Docs))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Poll task sources in their owning agent scopes

When task sources are created under a derived agent with a backend configured, the process-wide periodic poller and TaskSourcesConnectionSubscriber both call store::list_sources without that agent's scoped context (and the latter's child is also a bare tokio::spawn). This helper therefore enumerates only local outside SaaS or returns the no-acting-agent error in SaaS, so the agent's source records are never considered by either automatic path; schedule these consumers per agent or pass the owning scope explicitly through listing and fetch execution.

AGENTS.md reference: AGENTS.md:L401-L408

Useful? React with 👍 / 👎.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: 2 lane(s) blocking, worst finding is critical.

Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.

             $0.0422 · 1,132,412 in / 86,071 out · 77,495 cached (7%) · gpt-5.6-luna, glm-5.3-flash
critique:    $0.0342 · 516,113 in   / 39,326 out · 43,296 cached (8%) · gpt-5.6-luna, glm-5.3-flash
security:    $0.0062 · 436,326 in   / 30,796 out · 34,007 cached (8%) · gpt-5.6-luna
tests:       $0.0004 · 42,787 in    / 4,982 out  · 64 cached (0%)     · glm-5.3-flash
description: $0.0004 · 43,609 in    / 3,941 out  · 64 cached (0%)     · glm-5.3-flash
e2e:         $0.0004 · 46,241 in    / 3,372 out  · 64 cached (0%)     · glm-5.3-flash

fn sql_conv<E: std::fmt::Display>(err: E) -> rusqlite::Error {
rusqlite::Error::ToSqlConversionFailure(anyhow::anyhow!("{err}").into())
}
mod store_rows;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority critical critique confident

Add the declared store_rows module

This declaration has no corresponding store_rows.rs module in the repository, so the crate fails to compile with a missing module file. Add the module source at the expected path or remove the declaration and restore the definitions here.


Additional security observation

priority critical confident

Add the declared store_rows module

[RULE] missing-module-source

This declaration requires crates/openhuman-core/src/integrations/task_sources/store_rows.rs (or an inline module), but no such source is present in the provided pull-request diff. Unless that file is already present on the target branch, the crate will fail to compile because the module cannot be found. Add the module source containing these items or keep their definitions in this file.

[RULE] missing-module ·

}
}

#[cfg(test)]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority critical critique confident

Add the declared test module source

The new file declares store_documents_tests.rs under #[cfg(test)], but that source file is not included in the supplied patch. If it does not already exist in the branch, any test build fails with a missing module error. Add the sibling test file or remove the declaration.

[RULE] missing-module-source ·

let Some(stored) = docs.get(DEDUP, id).await? else {
return Ok(());
};
if stored.doc.get("last_ms").and_then(Value::as_i64) != Some(claimed_ms) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority high critique confident

Do not release another process's dedup claim

The rollback identifies ownership only by last_ms. Two callers can receive the same millisecond from Utc::now().timestamp_millis(); if the second caller claims the same content with skip_recent = false, the first caller's insert can fail and release_content will see the same timestamp and delete or restore the second caller's claim. A later retry can then be accepted as new even though the second notification was stored. Store a unique claim token/version in the dedup document and require that token when releasing the claim.

[RULE] atomic-claim-ownership ·

let id = n.id.clone();
let doc = to_doc(n);
let dedup = dedup_id(&n.provider, n.account_id.as_deref(), &n.title, &n.body);
let now_ms = Utc::now().timestamp_millis();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority high critique confident

Record the actual arrival time for deduplication

now_ms is captured before entering the document-store operation. Under executor or storage contention, the claim can be written substantially later, but its timestamp still reflects the earlier call time. This can make a notification appear older than it was and shorten the effective dedup window; capture the arrival/claim time immediately before the claim operation, or use the storage transaction's timestamp.

[RULE] stale-dedup-timestamp ·


fn collections() -> Vec<CollectionSpec> {
vec![
CollectionSpec::new(SOURCES).index(IndexSpec::new("by_created", ["created_ms"])),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium critique confident

Preserve full timestamp ordering when listing sources

created_ms truncates DateTime<Utc> to milliseconds. Two sources created within one millisecond are then ordered by _id, not by their actual creation time, so list_sources can violate insertion/creation order. Persist and sort by a lossless ordering key, or capture a deterministic insertion sequence for ties.

[RULE] lossy-ordering-key ·

triage_action: text(doc, "triage_action").map(str::to_string),
triage_reason: text(doc, "triage_reason").map(str::to_string),
status: status_of(text(doc, "status")),
received_at: parse_time(text(doc, "received_at")).unwrap_or_else(Utc::now),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium security confident

Do not replace invalid receipt times with the current time

A missing or malformed persisted received_at is replaced with the current time during reads. That changes historical notification metadata and can make old corrupt records appear newest, destabilizing listing and downstream processing. Propagate the parse error or explicitly omit/reject the malformed record.


Additional critique observation

priority medium confident

Do not treat malformed notification timestamps as valid state

[RULE] malformed-data-propagation

A corrupt received_at is silently replaced with the current time, changing the notification's ordering and making the corruption invisible to callers. A corrupt scored_at is silently converted to None, so stats and downstream logic treat a malformed scored notification as unscored. Return a storage/data error for malformed required timestamps, or explicitly preserve and surface the invalid-record state instead of normalizing it.


Additional e2e observation

priority medium confident

Do not replace invalid receipt times with the current time

[RULE] silent-parse-fallback

Still stands: an unparseable received_at becomes the current time, silently reordering and re-deduplicating against real arrivals. Surface the corruption rather than fabricating a timestamp.

[RULE] strict-timestamp-decoding ·


pub(super) fn list_sources(&self) -> Result<Vec<TaskSource>> {
let stored = self.0.run(|docs| async move {
let query = Query::all()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium security confident

Preserve full timestamp ordering when listing sources

Sources are persisted with only epoch milliseconds, so sources created within the same millisecond are ordered by _id rather than their actual DateTime<Utc> values. This can change the ordering promised by the SQLite implementation. Persist and sort by a precision-preserving timestamp key, or otherwise retain a deterministic full-precision ordering value.

[RULE] lossy-timestamp-ordering ·

}

pub(super) fn list_ingested_refs(&self, source_id: &str) -> Result<Vec<IngestedTaskRef>> {
let query = Query::filter(Filter::eq("source_id", source_id))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium security confident

Preserve full timestamp ordering for ingested references

The reference listing sorts only the millisecond timestamp and then the external ID. Multiple ingestions can share that millisecond while having different actual arrival times, so the document backend can return a different order from SQLite. Store and sort on a precision-preserving timestamp.

[RULE] lossy-timestamp-ordering ·

source_id: &str,
limit: usize,
) -> Result<Vec<NormalizedTask>> {
let query =

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium security confident

Preserve full ingestion ordering when listing tasks

Recently ingested tasks are ordered only by epoch milliseconds. Tasks received in the same millisecond are therefore ordered by the backend's unspecified tie behavior, which can violate the newest-first contract. Use a precision-preserving ingestion timestamp and an explicit stable tie-breaker.

[RULE] lossy-timestamp-ordering ·

triage_reason: text(doc, "triage_reason").map(str::to_string),
status: status_of(text(doc, "status")),
received_at: parse_time(text(doc, "received_at")).unwrap_or_else(Utc::now),
scored_at: parse_time(text(doc, "scored_at")),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium security confident

Do not treat malformed scoring timestamps as unscored

When scored_at is present but malformed, parse_time returns None, making the notification look unscored even though it has persisted scoring fields. This creates incorrect triage state and statistics. Distinguish a missing field from an invalid timestamp and propagate or reject the invalid record.


Additional e2e observation

priority medium confident

Do not treat malformed scoring times as unscored

[RULE] silent-parse-fallback

Still stands from the earlier revision and unchanged: a scored_at value that fails to parse makes the notification read as unscored, so the intelligence pipeline re-scores content it already scored. Return an error or preserve the raw string instead of silently dropping the field.

[RULE] strict-timestamp-decoding ·

@senamakel
senamakel merged commit 5e8ca60 into tinyhumansai:main Oct 9, 2026
18 of 24 checks passed
senamakel added a commit that referenced this pull request Oct 9, 2026
chore(ci): drop the saas-ambient baseline entry #7181 fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p0 Drop what you are doing. Data loss, a live break, or an exploitable hole.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant